fix: serve the SPA shell for ssr:false apps in rsbuild dev#76
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
commit: |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR enables dev-server setup for SPA mode, updates benchmark scripts and docs to match the new readiness behavior, and adds a Playwright dev-mode configuration plus e2e coverage for the SPA example. The example test script now runs both the default Playwright suite and the dev-mode suite. A changeset was added for a patch release noting SPA shell serving during 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts (1)
20-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo independent route checks combined in one test.
This test validates
/aboutand then separately navigates to/docs/getting-startedwithin the same test case. If the/aboutassertion fails, the nested-route coverage never runs, and failure diagnostics conflate two unrelated route checks.♻️ Split into separate test cases
- test('serves the SPA shell for deep links', async ({ page }) => { - const response = await page.goto('/about'); - - expect(response?.status()).toBe(200); - await expect(page.locator('h1:has-text("About This Demo")')).toBeVisible(); - - // Nested routes resolve as document requests too. - await page.goto('/docs/getting-started'); - await expect( - page.locator('h1:has-text("Getting Started")') - ).toBeVisible(); - }); + test('serves the SPA shell for deep links', async ({ page }) => { + const response = await page.goto('/about'); + + expect(response?.status()).toBe(200); + await expect(page.locator('h1:has-text("About This Demo")')).toBeVisible(); + }); + + test('serves the SPA shell for nested routes', async ({ page }) => { + await page.goto('/docs/getting-started'); + await expect( + page.locator('h1:has-text("Getting Started")') + ).toBeVisible(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts` around lines 20 - 31, The SPA deep-link coverage in test('serves the SPA shell for deep links') combines two independent route checks, so split the /about assertion and the /docs/getting-started assertion into separate test cases. Keep the existing page.goto and expect checks, but isolate each route into its own test so failures are independent and easier to diagnose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/spa-mode/playwright.dev.config.ts`:
- Around line 29-34: The Playwright webServer setup is only probing the dev
port, so the SPA can still race before the compiler is ready. Update the
Playwright dev config by changing the readiness check in the webServer block for
the dev server command/url to wait for the actual dev-ready signal from rsbuild
rather than only checking http://localhost:3002, keeping the existing webServer
configuration structure intact.
In `@tests/features.test.ts`:
- Around line 36-49: Clear the global test stub after the SPA middleware test so
it does not leak into later cases. In the `should register the dev middleware
for SPA mode (ssr: false)` test, clean up `globalThis.__reactRouterTestConfig`
after `pluginReactRouter()`/`createStubRsbuild()` assertions, using the same
`__reactRouterTestConfig` symbol to reset the state and keep tests isolated.
---
Nitpick comments:
In `@examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts`:
- Around line 20-31: The SPA deep-link coverage in test('serves the SPA shell
for deep links') combines two independent route checks, so split the /about
assertion and the /docs/getting-started assertion into separate test cases. Keep
the existing page.goto and expect checks, but isolate each route into its own
test so failures are independent and easier to diagnose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a8979cb-f831-4ede-81dd-dd2189270b5a
📒 Files selected for processing (7)
examples/spa-mode/package.jsonexamples/spa-mode/playwright.dev.config.tsexamples/spa-mode/tests/e2e-dev/spa-mode-dev.test.tsscripts/bench-builds.mtsscripts/benchmark/profiles.mjssrc/index.tstests/features.test.ts
💤 Files with no reviewable changes (1)
- scripts/benchmark/profiles.mjs
ScriptedAlchemy
force-pushed
the
claude/musing-shaw-924569
branch
from
a5b36d5 to
e400982
Compare
Rsbuild 2.1 deprecates `dev.setupMiddlewares` in favor of `server.setup`. Register the React Router dev middleware through a `server.setup` function that returns a post-built-in-middlewares callback, preserving the ordering guarantee that static assets are served before the request handler. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ScriptedAlchemy
force-pushed
the
claude/peaceful-benz-6eae2b
branch
from
b3c7fea to
7d2914b
Compare
SPA mode apps returned 404 for every route in dev because the plugin skipped its dev middleware when ssr:false and all web entries disable HTML generation, so no HTML document existed. React Router's request handler natively renders the SPA shell for ssr:false builds, and the dev server build (isSpaMode: true) is already compiled by the node environment, so register the middleware for SPA mode too. - Add a dev-mode Playwright suite to examples/spa-mode (separate config since the dev server writes into the build/ directory the static suite asserts against) - Remove the devRoutes: 'none' benchmark override from PR #75 and wait for the node environment before fetching SPA dev routes, since route serving now depends on the server build Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review cleanup: derive playwright.dev.config.ts from the sibling static config instead of copying it, reuse the testGlobal pattern in features.test.ts, fold the hydration-payload assertions into the shell test to drop a duplicate navigation, and remove the per-profile benchmark devRoutes override mechanism now that nothing produces it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The benchmark harness waits for an explicit, caller-provided set of dev environments (['web', 'node'] for every fixture variant — SPA dev route serving goes through the node server build too, so 'node' must be ready before route fetches). The ready-line pattern captures any environment name, and a startup timeout now reports which expected environments were still awaited vs. observed instead of hanging silently. Also aligns the SPA-mode feature test with the server.setup registration that now includes ssr:false apps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ScriptedAlchemy
force-pushed
the
claude/musing-shaw-924569
branch
from
e400982 to
b3fce8d
Compare
Contributor
Benchmark ResultsCompared PR head Dev Rollup
Production Build BenchmarksRendered 7 production build benchmarks.
full Dev Fixture SummaryRendered 7 dev benchmark fixtures from the
large-355-ssr-esm Plugin Operations
synthetic-1024-ssr-esm Plugin Operations
synthetic-1024-ssr-esm-split Plugin Operations
synthetic-256-sourcemaps Plugin Operations
synthetic-256-ssr-esm Plugin Operations
synthetic-256-ssr-esm-split Plugin Operations
synthetic-48-ssr-esm Plugin Operations
Synthetic Rsbuild AppRendered 2 production build benchmarks.
Rendered 1 dev benchmark fixture from the embedded complex app.
Profile: |
…o codex/pr-76-babysit
…o codex/pr-76-babysit
…o codex/pr76-changeset
# Conflicts: # src/index.ts # tests/features.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ssr: false(SPA mode) apps returned HTTP 404 for every route inrsbuild dev, including/. Two things combined to cause this:setupMiddlewaresskipped the dev request middleware wheneverssr: false(pluginOptions.customServer || !ssr ? [] : [...])html: false, so no HTML document existed in dev at allProduction worked because the SPA build prerenders an
index.htmlshell — and theexamples/spa-modee2e suite only testedbuild+ static serve, so this was never caught.Fix
React Router's request handler natively supports
ssr: falseserver builds: whenbuild.ssris false (and no prerender paths are configured) it renders the SPA shell for any document request — the same shell the production build prerenders intoindex.html. The dev server build (withisSpaMode: true) is already compiled by the node environment, which always exists. So the fix is to drop the!ssrexclusion and register the dev middleware for SPA mode too.Unknown paths return the app shell with a 404 status, so the client-side error boundary renders — matching React Router's Vite dev behavior.
Tests
ssr: false(tests/features.test.ts)examples/spa-mode(playwright.dev.config.ts+tests/e2e-dev/) covering/, deep links to nested routes, shell hydration data, and client-side navigation. Kept as a separate config because the dev server writes into the samebuild/directory the static suite asserts against;test:e2enow runs both, so CI picks it up.tsc --noEmitclean.Benchmarks
Removes the
devRoutes: 'none'workaround onsynthetic-256-spaadded in #75, so benchmarks measure SPA dev route serving again. Also waits for thenodeenvironment (not justweb) before fetching SPA dev routes, since route serving now goes through the server build — otherwise node compile time would be misattributed to route-fetch latency. Verified with a real run: all 8 dev route fetches return 200 (~20–165 ms), including the post-HMR-update fetch.🤖 Generated with Claude Code